Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] add a duration custom option type to support arbitrary duration units #21307

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Aug 15, 2024

Add a duration custom type for options (and companion class DurationOption) to support users specifying duration-typed values with a number and a unit. This will allow migrating existing options which takes milliseconds or seconds or some other unit to finally be consistent with one another.

E.g., the user can specify 10ms for 10 milliseconds or 50us for 50 microseconds.

@tdyas tdyas added category:new feature release-notes:not-required PR doesn't require mention in release notes labels Aug 15, 2024
@tdyas
Copy link
Contributor Author

tdyas commented Aug 15, 2024

@benjyw: How should I update the Rust option parser for this new option type? I see the following warning:

22:51:16.73 [WARN] Found differences between the new native options parser and the legacy options parser in scope [GLOBAL]:

- Failed to parse options with native parser due to error:
    The duration value `0:00:03` must be in `NUMBER UNIT` format.

@benjyw
Copy link
Contributor

benjyw commented Aug 16, 2024

TBH I think the options system types are already too complicated. As an alternative, maybe all durations could be integers representing milliseconds (do we ever actually need microseconds)? Or floats representing seconds? Or just strings with the interpretation done entirely on the consuming side in Python in some helper?

If you want to go down this path then the Rust side type should be String, with the interpretation done in Python (similar to what we do for addresses and so on).

@tdyas
Copy link
Contributor Author

tdyas commented Aug 18, 2024

TBH I think the options system types are already too complicated. As an alternative, maybe all durations could be integers representing milliseconds (do we ever actually need microseconds)? Or floats representing seconds? Or just strings with the interpretation done entirely on the consuming side in Python in some helper?

If you want to go down this path then the Rust side type should be String, with the interpretation done in Python (similar to what we do for addresses and so on).

You elude to my ulterior motive here: Uniform treatment from the user's perspective of all duration-typed options. We have some options which are seconds and some which are millis. Converting to timedelta on Python side is nice; but consistent user experience is my goal.

Do you consider allowing users to provide a duration string to be too much complication? As it is, users have to figure out what unit the option is using to use the options correctly. Allowing them to just specify what they want could be seen as less complicated. Thoughts?

@benjyw
Copy link
Contributor

benjyw commented Aug 19, 2024

What is the migration plan for existing options here - how would you interpret a naked integer? Fail on it after a deprecation period?

In any case, if you want to do this, I would not add a type for this in Rust, but have it be a String, and then interpret that string somewhere. Which is what we already do for all sorts of types like Address that are not explicitly handled in Rust.

@tdyas
Copy link
Contributor Author

tdyas commented Sep 13, 2024

What is the migration plan for existing options here - how would you interpret a naked integer? Fail on it after a deprecation period?

Migration thoughts:

  1. The default unit would be seconds since N would be interpreted the same as N.0 since floating point would also be accepted to specify fractional seconds. If we want the unit always specified, we could as you mention eventually introduce that as a requirement but need not do that.
  2. Existing options with "millis" or units in the name would be renamed (with appropriate deprecation cycle) to not have the unit in the option name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new feature release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants